[HOTFIX] - Fix broken review UI flow #1381
Conversation
…ing or encoding Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…ing or encoding Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Co-authored-by: kkartunov <5585002+kkartunov@users.noreply.github.com>
Fix ESLint semicolon violations in metadataMatching.ts
Potential fix for code scanning alert no. 71: Incomplete string escaping or encoding
Potential fix for code scanning alert no. 69: Incomplete string escaping or encoding
hotfix(PM-3208): add manager comment button showing to submitters and normal reviewer
Revert "fix(PM-2573): show only submissions with passed screening score"
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| )} | ||
|
|
||
| {!showCommentForm && !comment && isManagerEdit && ( | ||
| {!showCommentForm && !comment && isManagerEdit && canAddManagerComment && ( |
There was a problem hiding this comment.
[correctness]
The addition of canAddManagerComment to the condition ensures that the button to add a manager comment is only shown when it is allowed. Verify that canAddManagerComment is correctly set in all contexts where this component is used to avoid unexpected behavior.
| isAppealsTab: false, | ||
| }), | ||
| [], | ||
| [canRespondToAppeals], |
There was a problem hiding this comment.
[performance]
The useMemo dependency array now includes canRespondToAppeals, which is correct for ensuring the memoized value updates when canRespondToAppeals changes. However, ensure that canRespondToAppeals is a stable value and doesn't change unnecessarily, as this could lead to unnecessary re-renders.
| <Link | ||
| className={styles.respondButton} | ||
| to={getReviewRoute(submission.id, reviewId)} | ||
| to={getReviewRoute(submission.id, reviewId, canRespondToAppeals)} |
There was a problem hiding this comment.
[❗❗ correctness]
The addition of canRespondToAppeals as a parameter to getReviewRoute suggests that the route logic might change based on this flag. Ensure that this change is backward compatible and that all parts of the application using this route are updated accordingly to handle the new parameter.
| ...aggregated.submission, | ||
| aggregated, | ||
| })) as SubmissionRow[] | ||
| const rows = aggregatedSubmissionRows.map(aggregated => ({ |
There was a problem hiding this comment.
[❗❗ correctness]
The filtering logic for myReviewDetail has been removed. This could lead to incorrect data being processed if the filtering was necessary to exclude certain submissions. Ensure that this change does not introduce any logical errors or unintended behavior.
| const reviewUrl = getReviewUrl ? getReviewUrl(reviewId) : getReviewRoute(submission.id, reviewId) | ||
| const reviewUrl = getReviewUrl | ||
| ? getReviewUrl(reviewId) | ||
| : getReviewRoute(submission.id, reviewId, canRespondToAppeals) |
There was a problem hiding this comment.
[❗❗ correctness]
The getReviewRoute function now takes an additional canRespondToAppeals parameter. Ensure that this function handles the new parameter correctly and that all usages of getReviewRoute are updated accordingly. This change could impact the correctness of the URL generation.
| const reviewUrl = getReviewUrl ? getReviewUrl(reviewId) : getReviewRoute(submission.id, reviewId) | ||
| const reviewUrl = getReviewUrl | ||
| ? getReviewUrl(reviewId) | ||
| : getReviewRoute(submission.id, reviewId, canRespondToAppeals) |
There was a problem hiding this comment.
[❗❗ correctness]
The getReviewRoute function now takes an additional canRespondToAppeals parameter. Ensure that this function handles the new parameter correctly and that all usages of getReviewRoute are updated accordingly. This change could impact the correctness of the URL generation.
| const targetWithPlaceholder = target.replace(sepPattern, WORDSEP_PLACEHOLDER) | ||
| // Properly escape ALL regex metacharacters (including backslash), leaving the placeholder intact | ||
| const escapedTarget = escapeRegexLiteral(targetWithPlaceholder) | ||
| .replace(new RegExp(escapeRegexLiteral(WORDSEP_PLACEHOLDER), 'g'), '[-_\\s]+') |
There was a problem hiding this comment.
[correctness]
The use of [-_\\s]+ to replace the placeholder could potentially match unintended sequences if the target string contains special characters that are not properly escaped. Consider reviewing the escapeRegexLiteral function to ensure it handles all edge cases, or explicitly document the expected input constraints.
| export function getReviewRoute( | ||
| submissionId: string, | ||
| reviewId: string, | ||
| addRespondToAppeals?: boolean, |
There was a problem hiding this comment.
[💡 readability]
Consider renaming addRespondToAppeals to includeRespondToAppeals for clarity, as it better describes the boolean nature of the parameter.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-3207 and related issues.
What's in this PR?